fix(storage): add retry support for finalize and close in AsyncAppendableObjectWriter - #17733
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds support for custom and default retry policies during the finalization and closing of an asynchronous appendable object writer, along with corresponding unit tests. However, the current implementation of the retry mechanism in finalize is flawed because it attempts to reuse the same gRPC stream (write_obj_stream) after an error has occurred. In production, a gRPC stream enters an errored state and closes upon failure, meaning subsequent retries on the same stream will fail immediately. To resolve this, the underlying stream must be re-established or recreated before retrying.
208879e to
3366eb0
Compare
b509e2e to
2bc1f95
Compare
…ableObjectWriter Fixes: b/532527637
2bc1f95 to
b2ce145
Compare
|
@sahusneha2004 unit tests are failing. Add Description as well. |
| if ( | ||
| self.offset is not None | ||
| and expected_offset is not None | ||
| and self.offset < expected_offset |
There was a problem hiding this comment.
make it != instead of < . There could be a scenario where another writer might have written data to the same object
There was a problem hiding this comment.
make the same change in _do_finalize() as well.
|
|
||
| assert res == resource | ||
| assert writer.persisted_size == 999 | ||
| assert mock_appendable_writer["mock_stream"].send.await_count == 2 |
There was a problem hiding this comment.
add await_count on recvassert mock_appendable_writer["mock_stream"].recv.await_count == 2
| ].recv.return_value = storage_type.BidiWriteObjectResponse(resource=resource) | ||
|
|
||
| res = await writer.finalize(retry_policy=custom_policy) | ||
| assert res == resource |
There was a problem hiding this comment.
what about other other assert statements , similar to above test case ? test_finalize_retry_on_transient_error
| writer.finalize.assert_awaited_once_with( | ||
| full_object_checksum=None, | ||
| retry_policy=custom_policy, | ||
| ) |
There was a problem hiding this comment.
add more assert statements similar to onces in test_close_retry_on_transient_error
|
@sahusneha2004 merge this PR only after googleapis/storage-testbench#811 is merged. |
2 participants
This PR introduces robust retry support for the close() and finalize() operations on bidirectional gRPC appendable streams (AsyncAppendableObjectWriter). Previously, if a transient error (such as a 503 Unavailable or a redirect error) occurred exactly when sending the finish_write signal, the stream would fail without attempting to recover.
This change ensures that close() and finalize() respect the configured retry_policy and can seamlessly reconnect and retry the finalization if a transient failure or backend redirection occurs.
Key Changes:
Added Retry Loop in Finalize: Refactored finalize() to wrap the gRPC requests_done() and response logic inside a new _do_finalize() helper function. This function uses a retry loop to catch transient exceptions.
Stream Re-initialization on Retry: If a retriable error occurs during finalize, _do_finalize() will safely attempt to re-open the stream via await self.open(metadata=metadata) and re-send the finish_write=True signal to successfully close out the object.
Metadata Pass-through: Added an optional metadata parameter to both close() and finalize(). This ensures that any custom gRPC headers (such as x-retry-test-id used by the Storage Testbench) are preserved and correctly passed to self.open() if a retry connection needs to be established.
Testing:
Unit Tests: Updated tests/unit/asyncio/test_async_appendable_object_writer.py to assert the new metadata argument signatures and verify the retry logic mock calls.
Integration Tests: Added new conformance test scenarios in tests/conformance/test_bidi_writes.py ("name": "Retry on 503 during close/finalize") utilizing the empty_write: True flag. This successfully verifies the retry mechanisms against the Storage Testbench emulator, including both 503 Unavailable and BidiWriteObjectRedirectedError (Redirect Error) faults during finalization.
Fixes: b/532527637